Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CS2] Support await in REPL without wrapper function #4604

Merged
merged 2 commits into from
Jul 24, 2017
Merged

[CS2] Support await in REPL without wrapper function #4604

merged 2 commits into from
Jul 24, 2017

Conversation

connec
Copy link
Collaborator

@connec connec commented Jul 5, 2017

I went ahead and made a fix for #4603 because I think it could be a really useful feature for playing with promise libraries.

I took care to ensure non-await evaluations will still happen fully synchronously (e.g. process.nextTick -> console.log 'async' works as before).

I didn't write any tests, because putting them in test/repl.coffee or test/async.coffee would involve quite a bit of copypasta. I did think of making the async test globally available (e.g. as test.async or testAsync) but that was a bigger change than I'd planned to make. If someone has an opinion on how to deal with it I'm happy to add some tests.


Rather than compiling top-level await expressions directly, which will
always throw a syntax error, the REPL wrapper will now wrap them in a
closure and handle the returned promise before calling back to the REPL.

Fixes #4603.

@lydell
Copy link
Collaborator

lydell commented Jul 5, 2017

How come you could implement this so easily, while nodejs/node#13209 is still open? Some kind of backwards compatibility thing? What am I missing?

@connec
Copy link
Collaborator Author

connec commented Jul 5, 2017

Interesting... it seems a lot in that thread is to do with ambiguity with await as a variable (which isn't something CS needs to deal with as await is always parsed as an 'operator') as well as confusion between REPL support and supporting top-level await (which is a whole other thang). Perhaps it is easy in this case because we're using a custom evaluator, whereas the default evaluator just uses eval? I'm not sure how the internals work 😞

One thing I didn't think about that came up in the thread is what should happen if the user ctrl+c's an awaiting promise. The way this is implemented right now, if the promise hangs forever there is no observable issue (same as doing new Promise -> in the REPL), however if the promise eventually resolves (e.g. await new Promise (r) -> setTimeout r, 5000) then the REPL will eventually log the result (in this case undefined), even after ctrl+c is pressed.

screen shot 2017-07-05 at 19 50 45

That said, with some acrobatics it should be possible to intercept the SIGINT and prevent the REPL handler from being called to print the result (which won't prevent the computation from running of course, which is the nature of async in JS).

@GeoffreyBooth GeoffreyBooth changed the title Support await in REPL without wrapper function [CS2] Support await in REPL without wrapper function Jul 9, 2017
@connec
Copy link
Collaborator Author

connec commented Jul 13, 2017

@AlvaroBernalG do you want to have a play with this branch and see if it works as you'd hoped?

@AlvaroBernalG
Copy link

@connec I have been playing with your branch and the only thing that I found is that I am unable to assign the result of an await to a variable. Which useful in cases when you want to comfortably inspect the element returned.

@princejwesley has a gist where he solve a similar problem by appending the assignment variable to
the global object.

So if you have an await whose result is being assigned to a variable like so:

coffee>myVariableResult = await wiki().page('batman')

It would get transpiled into:

// `(async() => global.${myVariableResult}=${awaitExpression})()`

(async() => global.myVariableResult = await wiki().page('batman'))()

if there is no variable assignment then:

// `(async() => ${awaitExpression})()`

(async() => await wiki().page('batman'))()

@lydell
Copy link
Collaborator

lydell commented Jul 14, 2017

@AlvaroBernalG Good example. That's certainly unexpected behavior.

@connec
Copy link
Collaborator Author

connec commented Jul 14, 2017

Good catch indeed, didn't think about assignment. I suppose I need to update the isAwait condition to also accept Assign's whose value is an Await.

Just to be clear, the second example ("if there is no variable assignment") is working as expected?

@AlvaroBernalG
Copy link

@connec I confirm that besides that everything is working as expected ( Even canceling the promise before it finish with ctrl +c .. which is awesome :) )

@connec
Copy link
Collaborator Author

connec commented Jul 14, 2017

I found a much more robust approach that also supports await inside arbitrary expressions, e.g.

console.log await Promise.resolve 1

# and assignment, of course :)
foo = await Promise.resolve 1

About the cancelling... that's actually a bit weird, and I don't think there's anything we can easily do about it. For example, running the following in the REPL will result in foo being set to the 'cancelled' value (1).

foo = await new Promise (resolve) -> setTimeout (-> resolve 1), 5000
# ^C
foo = 2
# wait a few seconds...
_
# undefined
foo
# 1

This is because it's impossible to actually cancel a native promise.

That said, in its current form it's useful for playing with promise libraries, and the same problem would exist in manual usage:

foo = null
(new Promise (resolve) -> setTimeout (-> resolve 1), 5000).then (result) -> foo = result
foo = 2
# wait a few seconds
foo
# 1

Let me know if this works better for you @AlvaroBernalG 😄

Rather than compiling top-level await expressions directly, which will
always throw a syntax error, the REPL wrapper will now wrap them in a
closure and handle the returned promise before calling back to the REPL.

Fixes #4603.
This change allows users to 'cancel' awaited expressions that they don't
want to see the result of. Ultimately, this *does not* prevent the
expression from completing (e.g. for `await fetch(url)`, `url` will
still be downloaded, but the user won't have to wait for it to finish
and the result won't be printed).
@GeoffreyBooth
Copy link
Collaborator

@AlvaroBernalG can you please confirm that this PR works as expected?

@lydell, any notes or is this ready to merge?

@lydell
Copy link
Collaborator

lydell commented Jul 24, 2017

I haven't tested it, but the code looks good.

@AlvaroBernalG
Copy link

@connec @GeoffreyBooth Apologies for the delay. For some reason I missed the emails and warnings on Github.

I tested the branch and I can confirm that it is working as expected.

@connec Thank you very much. Coffeescript REPL is now my default REPL.

Once again apologies for the delay.

@GeoffreyBooth
Copy link
Collaborator

Great, thanks!

@GeoffreyBooth GeoffreyBooth merged commit 6c9cf37 into jashkenas:2 Jul 24, 2017
@connec connec deleted the #4603 branch July 25, 2017 21:56
@connec
Copy link
Collaborator Author

connec commented Jul 25, 2017

Glad it's working out for you @AlvaroBernalG 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants